Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HMS-4934: Add modules to introspected data #922

Merged
merged 6 commits into from
Jan 17, 2025
Merged

HMS-4934: Add modules to introspected data #922

merged 6 commits into from
Jan 17, 2025

Conversation

jlsherrill
Copy link
Member

@jlsherrill jlsherrill commented Dec 16, 2024 •
edited
Loading

Summary

Adds module streams as searchable for introspected repos

Testing steps

  1. Adjust go.mod to point to your localy checkout of yummy, which should have HMS-4934: Parse module metadata yummy#31
replace github.com/content-services/yummy => /home/jlsherri/git/yummy/
  1. go get ./...
  2. make db-migrate-up
  3. make repos-import
  4. go run cmd/external-repos/main.go introspect go run cmd/external-repos/main.go introspect https://cdn.redhat.com/content/dist/rhel8/8/x86_64/appstream/os
  5. go run cmd/external-repos/main.go introspect go run cmd/external-repos/main.go introspect https://cdn.redhat.com/content/dist/rhel9/9/x86_64/appstream/os
  6. play around with the module streams search api:
#### module streams
POST http://localhost:8000/api/content-sources/v1.0/module_streams/search
x-rh-identity: eyJpZGVudGl0eSI6eyJ0eXBlIjoiVXNlciIsInVzZXIiOnsidXNlcm5hbWUiOiIxIn0sImFjY291bnRfbnVtYmVyIjoiZm9vIiwiaW50ZXJuYWwiOnsib3JnX2lkIjoiMSJ9fX0K
Content-Type: application/json

{"urls":["https://cdn.redhat.com/content/dist/rhel8/8/x86_64/appstream/os"],
  "search":"",
  "rpm_names": []
}

feel free to play with the search and rpm_names

@jlsherrill
Copy link
Member Author

https://issues.redhat.com/browse/HMS-4932

@jlsherrill jlsherrill changed the title Fixes 4932: Add search module streams Fixes HMS-4932: Add search module streams Dec 17, 2024
@jlsherrill jlsherrill marked this pull request as ready for review December 17, 2024 22:16
@jlsherrill

This comment was marked as outdated.

Sign in to view
@jlsherrill jlsherrill marked this pull request as draft December 18, 2024 17:19
@jlsherrill jlsherrill changed the title Fixes HMS-4932: Add search module streams Fixes HMS-4934: Add modules to introspected data Dec 18, 2024
@jlsherrill

This comment was marked as outdated.

Sign in to view
@jlsherrill

This comment was marked as outdated.

Sign in to view
@jlsherrill jlsherrill changed the title Fixes HMS-4934: Add modules to introspected data HMS-4934: Add modules to introspected data Dec 18, 2024
@jlsherrill
Copy link
Member Author

https://issues.redhat.com/browse/HMS-4934

@jlsherrill jlsherrill marked this pull request as ready for review January 7, 2025 21:01
@swadeley
Copy link
Member

swadeley commented Jan 8, 2025

/retest

@swadeley
Copy link
Member

swadeley commented Jan 8, 2025

@jlsherrill this failed to build

@jlsherrill
Copy link
Member Author

jlsherrill commented Jan 8, 2025 •
edited
Loading

@swadeley yes, it will fail to build until content-services/yummy#31 is merged and i update it to use the new yummy

@rverdile rverdile self-assigned this Jan 10, 2025
rverdile
rverdile reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@rverdile rverdile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I search for a stream that doesn't exist, I see null as the response. Would empty list be better? I think that's what I would've expected

@jlsherrill
Copy link
Member Author

good call, fixed

rverdile
rverdile reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@rverdile rverdile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed if I create and introspect a repository before checking-out this PR, then checkout this PR and re-introspect, the module streams don't get added because the repository hasn't changed (I suspect)

@jlsherrill
Copy link
Member Author

good call, updated!

rverdile
rverdile reviewed Jan 15, 2025
View reviewed changes
db/migrations/20250107150808_AddModuleStreams.up.sql Outdated
@@ -45,4 +45,6 @@ ALTER TABLE ONLY module_streams
DROP CONSTRAINT IF EXISTS fk_module_streams_uniq,
ADD CONSTRAINT fk_module_streams_uniq UNIQUE (hash_value);

UPDATE repositories SET repomd_checksum = '' where public = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works, so for non public repos the user may have to recreate the repo? This is probably an edge so maybe it's not a big deal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe force introspecting the repo should suffice if needed.

In reality, the only 3rd party repo i know that has modules is nvidia's which gets regular updates, so by the time image builder actually support modules, i imagine it would be refreshed. Or we could manually force its introspection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think force introspecting only forces the task to run, but the task itself will still exit early. But it sounds like this isn't a realistic issue so I agree it should be fine

@rverdile
Copy link
Contributor

acked the yummy PR! Small question unrelated to the yummy changes but looks good

@swadeley
Copy link
Member

Hi

testing in ephemeral first with : https://rverdile.fedorapeople.org/dummy-repos/modules/repo1/

(will test introspected repo in stage)

In [7]: app.content_sources.rest_client.module_streams_api.search_snapshot_module_streams(dict(search='walrus', uuids=['177e9b6f-0125-4628-9953-40195e368822'], rpm_names=[]))
2025-01-16 11:52:41.209 [    INFO] [iqe.base.rest_client] REST: POST https://ee-fryb0ek4.apps.crc-eph.r9lp.p1.openshiftapps.com/api/content-sources/v1/snapshots/module_streams/search with query params [] and x-rh-insights-request-id=<>
Out[7]: 
[{'module_name': 'walrus',
  'streams': [{'arch': 'x86_64',
               'context': 'c0ffee42',
               'description': 'A module for the walrus 0.71 package',
               'name': 'walrus',
               'profiles': {'default': ['walrus'], 'flipper': ['walrus']},
               'stream': '0.71',
               'version': '20180707144203'},
              {'arch': 'x86_64',
               'context': 'deadbeef',
               'description': 'A module for the walrus 5.21 package',
               'name': 'walrus',
               'profiles': {'default': ['walrus']},
               'stream': '5.21',
               'version': '20180704144203'}]}]

@swadeley swadeley requested a review from rverdile January 16, 2025 04:28
@swadeley
Copy link
Member

Hi @jlsherrill , please rebase

@swadeley swadeley merged commit d8c1e99 into content-services:main Jan 17, 2025
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rverdile rverdile rverdile approved these changes

Assignees

@rverdile rverdile

Labels
qe-testing-needed Ready for QE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jlsherrill @swadeley @rverdile